[Woo POS] Update conditions for showing POS entry point #11513
Conversation
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/IsWooPosEnabled.kt
Show resolved
Hide resolved
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
|
Tested on tablet, phone and split screen: 
```html
```html
|
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/IsWooPosEnabled.kt
Outdated
Show resolved
Hide resolved
…if-pos-mode-available-show-entry-point-only-if-it-allows-it
| private val isWindowSizeExpandedAndBigger: IsWindowClassExpandedAndBigger, | ||
| ) { | ||
| @Suppress("ReturnCount") | ||
| suspend operator fun invoke(): Boolean { |
There was a problem hiding this comment.
Another important thing to notice - we have to check the flag first, otherwise all these checks will be performed even on the production build making it slower for no reason at the moment
There was a problem hiding this comment.
Even after we check FF first in here, we may still need to create a ticket to consider early background check + caching here. Currently, we do network requests, and the whole "more" screen doesn't show anything. Depending on the entry point we probably will need to reconsider the approach
There was a problem hiding this comment.
@samiuelson
it's not really needed because the state of the UI is cached in the ViewModel anyway, so it's kinda fetched once anyway, but that first time is looking bad. So you can keep it or revert - doesn't matter, but the most important that you moved isWooPosFFEnabled here so on the release builds we won't slow this down
There was a problem hiding this comment.
@samiuelson Oh I see. The VM initiated even before the fragment is opened. Then the caching probably makes sense even for debug builds - we just need to keep in mind that the app will have to be killed in case of a site change. I'd add a ticket to come back to this closer to the release - we will need cache invalidation here
There was a problem hiding this comment.
Well, now the menu more screen should load faster even the first time because the IsWooPosEnabled-related requests are now executed in the MainActivityViewModel::init – right after the user launches the app (or logs in) and the result is stored in the memory (IsWooPosEnabled is made a singleton).
There was a problem hiding this comment.
Cool – I created the issue to test cache invalidation need later – #11520
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/IsWooPosEnabled.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@samiuelson thank you for your work!
I left a few comments, and I believe some of them are important to address; please take a look
| @@ -89,3 +89,7 @@ object UiHelpers { | |||
| class IsWindowClassLargeThanCompact @Inject constructor(val context: Context) { | |||
| operator fun invoke() = context.windowSizeClass != WindowSizeClass.Compact | |||
| } | |||
|
|
|||
| class IsWindowClassExpandedAndBigger @Inject constructor(val context: Context) { | |||
| operator fun invoke() = context.windowSizeClass == WindowSizeClass.ExpandedAndBigger | |||
There was a problem hiding this comment.
Do you know if small tablets or big phones e.g. 7-8 inch will be covered by that? Do we actually plan to support 7-8inch size devices?
There was a problem hiding this comment.
The current condition targets an 8" or larger diagonal (assuming a standard 16:10 ratio). I added a ticket to discuss it and adjust final values later - #11522
…if-pos-mode-available-show-entry-point-only-if-it-allows-it
This reverts commit 7806925.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11513 +/- ##
============================================
+ Coverage 40.45% 40.46% +0.01%
- Complexity 5181 5191 +10
============================================
Files 1082 1083 +1
Lines 62904 62925 +21
Branches 8616 8626 +10
============================================
+ Hits 25447 25463 +16
- Misses 35169 35171 +2
- Partials 2288 2291 +3 ☔ View full report in Codecov by Sentry. |
This reverts commit 65c0098.
samiuelson
deleted the
11498-woo-pos-build-a-class-that-determine-if-pos-mode-available-show-entry-point-only-if-it-allows-it
branch
Closes: #11498
Description
This PR updates the conditions for showing the POS entry point:
Testing instructions
Ensure the POS entry point button (in the menu more screen) is visible if the above conditions are met and invisible otherwise.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.